Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Base.isless for sorting AbstractModelAttribute during MOI.copy_to #2369

Closed
wants to merge 1 commit into from

Conversation

odow
Copy link
Member

@odow odow commented Dec 20, 2023

Closes #2366

Thoughts on something like this?

An alternative would be to expand on the _sort_priority(::Any) = 2 to have copy_to_sort_priority(::ModelLike, ::AbstractModelAttribute), but then this gets a bit messy if we have to have extensions working together.

@joaquimg
Copy link
Member

Would a change in priorities be considered breaking?

@joaquimg
Copy link
Member

Also,
Do you envision different solvers having different priorities?
In this case the should have their own copy_to sorting with their priorities, and this default copy_to would be fine.

@odow
Copy link
Member Author

odow commented Dec 20, 2023

I think so. #2366 is really asking for a first-class priority order of attributes.

Perhaps Base.isless should be instead some official MOI.CopyToPriorityFactor or something.

But open questions are whether it should depend on the dest::MOI.ModelLike, or whether there is some solver-independent ordering.

The attributes really form a DAG, and we need a topological sort of them... It'd be nice to say that ObjectiveFunction depends on ObjectiveSense, and then have the order fall out of that.

@blegat
Copy link
Member

blegat commented Dec 20, 2023

We could maybe also have SortedListOfModelAttributes that could be used for solvers so that they don't need to worry about the sort. MOI.Utilities.Model could also implement a getter for this attribute quite easily.
Maybe we can also require solvers to always return it sorted but maybe an explicit sort here is safer.

@odow
Copy link
Member Author

odow commented Dec 20, 2023

SortedListOfModelAttributes is reasonable. But it means that it works only for attributes defined in MOI proper. Everything else would come at the end in an arbitrary order.

@odow
Copy link
Member Author

odow commented Dec 20, 2023

#2371 is a much cleaner approach.

@odow
Copy link
Member Author

odow commented Dec 20, 2023

Closing in favor of #2371

@odow odow closed this Dec 20, 2023
@odow odow deleted the od/sort-attributes branch December 20, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Order of model attributes during copy
3 participants